Add add_foreign_utxo by LLFourn · Pull Request #279 · bitcoindevkit/bdk

LLFourn · 2021-02-08T05:11:55Z

Description

Fixes #114

This adds a method to TxBuilder called add_foreign_utxo which takes an outpoint, a psbt::Input and a satisfaction weight.
I decided taking a psbt::Input was the most flexible way to proceed and this seems to be the most compatible way of implementing BIP78 (well technically add_psbt would be closer to what they're going for -- this could be done more easily after this PR).

Misc changes

UTXO has been renamed to LocalUtxo.
TxBuilder wasn't properly Clone before. I've fixed this and used it in a test.
(UTXO, usize) has been replaced with WeightedUtxo -- no particular motivation it was just annoying me.

Notes to the reviewers

Pay attention to my doc comments and see if they can be improved.
I would have liked to avoiding having Utxo as an enum with two variants but normalizing both foreign and local utxos into the same struct seemed to be more complicated.
Is there anything you can see that is not covered by the test cases?

Checklists

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing
I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md
This pull request breaks the existing API
I'm linking the issue being fixed by this PR

tcharding

My review is not super deep in line with my knowledge of the codebase but here are a few minor suggestions for you.

Thanks

src/wallet/mod.rs

src/wallet/tx_builder.rs

afilini · 2021-02-09T18:21:01Z

I would have liked to avoiding having Utxo as an enum with two variants but normalizing both foreign and local utxos into the same struct seemed to be more complicated.

I've been thinking about this a bit but I don't really have decent any suggestions. I guess you could make Utxo a trait and implement it on ForeignUtxo and WeightedUtxo, but I don't like that solution too much. More generics around and it could get tricky because ForeignUtxo doesn't actually contain all the information required to spend it because we take them from the descriptor.

I gave this a quick look this morning and to be honest I couldn't really find any issues. I'll give it a more in-depth review now but I'm not expecting to find anything major, so we might be on track to merge this today/tomorrow just in time for the feature freeze.

src/types.rs

LLFourn · 2021-02-10T01:37:16Z

I would have liked to avoiding having Utxo as an enum with two variants but normalizing both foreign and local utxos into the same struct seemed to be more complicated.

I've been thinking about this a bit but I don't really have decent any suggestions. I guess you could make Utxo a trait and implement it on ForeignUtxo and WeightedUtxo, but I don't like that solution too much. More generics around and it could get tricky because ForeignUtxo doesn't actually contain all the information required to spend it because we take them from the descriptor.

Let me elaborate on the difficulty I had because it's not fundamental. I tried to add a function to wallet like get_utxo_as_psbt_input(outpoint: OutPoint) -> Result<Option<psbt::Input>, Error>. Then I could use that in add_utxo to get the psbt::Input and therefore normalize local UTXOs into the same type as foreign utxos.

The only problem was dealing with force_non_witness_utxo. If I create early in the builder I can't know whether to include non_witness_utxo.

To work around this I thought there could be a private enum type in TxBuilder but then before we do coin selection we could normalize it into a psbt::Input type now that we know force_non_witness_utxo. Ok but then we have to populate a psbt::Input for every input in the wallet. This could require significant time (I think) for a wallet with a lot of UTXOs.

So in the end I think we shouldn't try and normalize foreign and local utxos into the same type per-maturlely and that an enum is the right way to go.

Hopefully that makes sense.

LLFourn · 2021-02-11T23:53:17Z

I addressed comments so far and pushed a commit to improve the test code a bit.

Now clippy is upset at me for I didn't write. Help!

notmandatory · 2021-02-12T00:10:54Z

Now clippy is upset at me for I didn't write. Help!

I have #282 to fix the clippy errors, they're caused by the Rust stable version changing again.

notmandatory · 2021-02-16T18:45:43Z

Sorry this needs to be rebased again to pick up the PR that just went in to change the CI pipeline from stable to 1.50.0.

LLFourn · 2021-02-17T10:18:01Z

The build is failing because some rust infra is down -- will try again tomorrow.

Since this struct has a "keychain" it is not a general "UTXO" but a local wallet UTXO.

it derived Clone but in practice it was never clone because some of the parameters were not Clone.

To allow adding UTXOs external to the current wallet. The caller must provide the psbt::Input so we can create a coherent PSBT at the end and so this is compatible with existing PSBT workflows. Main changes: - There are now two types of UTXOs, local and foreign reflected in a `Utxo` enum. - `WeightedUtxo` now captures floating `(Utxo, usize)` tuples - `CoinSelectionResult` now has methods on it for distinguishing between local amount included vs total.

Co-authored-by: Tobin C. Harding <me@tobin.cc>

Noticed some suboptimal things while reviewing myself.

afilini

🚀

tcharding reviewed Feb 9, 2021

View reviewed changes

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Show resolved Hide resolved

src/wallet/tx_builder.rs Outdated Show resolved Hide resolved

src/wallet/tx_builder.rs Outdated Show resolved Hide resolved

afilini reviewed Feb 9, 2021

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

This was referenced Feb 11, 2021

Set merge_imports = true in rustfmt.toml #283

Closed

Turn on rustfmt merge_imports #284

Closed

LLFourn force-pushed the add_foreign_utxo3 branch from 8ae3100 to 899fe58 Compare February 17, 2021 10:15

LLFourn force-pushed the add_foreign_utxo3 branch from 899fe58 to ad090db Compare February 20, 2021 04:56

LLFourn and others added 6 commits February 26, 2021 13:33

s/UTXO/LocalUtxo/g

a7183f3

Since this struct has a "keychain" it is not a general "UTXO" but a local wallet UTXO.

Make TxBuilder actually Clone

9a918f2

it derived Clone but in practice it was never clone because some of the parameters were not Clone.

Apply typo fixes from @tcharding

7c1dcd8

Co-authored-by: Tobin C. Harding <me@tobin.cc>

Improve docs of satisfaction_weight

d9a102a

Clean up add_foreign_utxo tests a bit

ac7c1bd

Noticed some suboptimal things while reviewing myself.

LLFourn force-pushed the add_foreign_utxo3 branch from ad090db to ac7c1bd Compare February 26, 2021 02:34

afilini approved these changes Feb 26, 2021

View reviewed changes

afilini merged commit e0183ed into bitcoindevkit:master Feb 26, 2021

LLFourn deleted the add_foreign_utxo3 branch March 1, 2021 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add add_foreign_utxo#279

Add add_foreign_utxo#279
afilini merged 6 commits intobitcoindevkit:masterfrom
LLFourn:add_foreign_utxo3

LLFourn commented Feb 8, 2021

Uh oh!

tcharding left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afilini commented Feb 9, 2021

Uh oh!

Uh oh!

LLFourn commented Feb 10, 2021

Uh oh!

LLFourn commented Feb 11, 2021

Uh oh!

notmandatory commented Feb 12, 2021

Uh oh!

notmandatory commented Feb 16, 2021

Uh oh!

LLFourn commented Feb 17, 2021

Uh oh!

afilini left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

LLFourn commented Feb 8, 2021

Description

Misc changes

Notes to the reviewers

Checklists

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afilini commented Feb 9, 2021

Uh oh!

Uh oh!

LLFourn commented Feb 10, 2021

Uh oh!

LLFourn commented Feb 11, 2021

Uh oh!

notmandatory commented Feb 12, 2021

Uh oh!

notmandatory commented Feb 16, 2021

Uh oh!

LLFourn commented Feb 17, 2021

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants